Shutterized Dispute Kit #1965
Shutterized Dispute Kit #1965
Conversation
WalkthroughThis update adds the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShutterScript (CLI)
participant Shutter API
participant Shutter SDK
User->>ShutterScript: encrypt <message>
ShutterScript->>Shutter API: fetchShutterData(decryptionTimestamp)
Shutter API-->>ShutterScript: {pubkey, params, identity}
ShutterScript->>Shutter SDK: encryptData(message, pubkey, params, sigma)
Shutter SDK-->>ShutterScript: encryptedCommitment
ShutterScript-->>User: {encryptedCommitment, identity}
User->>ShutterScript: decrypt <encryptedMessage> <identity>
ShutterScript->>Shutter API: fetchDecryptionKey(identity)
Shutter API-->>ShutterScript: decryptionKey
ShutterScript->>Shutter SDK: decrypt(encryptedMessage, decryptionKey)
Shutter SDK-->>ShutterScript: decryptedMessage
ShutterScript-->>User: decryptedMessage
sequenceDiagram
participant Core
participant DisputeKitClassic
participant DisputeKitShutter
participant Subgraph
Core->>DisputeKitClassic: createDispute(...)
Core->>DisputeKitShutter: createDispute(...)
DisputeKitClassic-->>Core: DisputeKitCreated(id=1)
DisputeKitShutter-->>Core: DisputeKitCreated(id=2)
Core-->>Subgraph: DisputeKitCreated events
Subgraph->>DisputeKitClassic: handle events (dynamic kit ID)
Subgraph->>DisputeKitShutter: handle events (dynamic kit ID)
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jaybuidl
added
Type: Feature🗿
Package: Web
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
439-453: Added virtual hashVote function for extensible vote hashingThis new
hashVotefunction is a key architectural change that enables the Shutter integration:
- It's declared as
virtualso derived contracts can override it- The base implementation hashes only
_choiceand_salt(backward compatible)- It accepts but doesn't use the
_justificationparameter in the base implementation- The comment explicitly indicates that derived contracts may use the unused parameters
This design enables the specialized Shutter implementation to include the justification in the hash calculation.
This is a well-designed extension point that maintains backward compatibility while enabling new functionality in derived contracts. The virtual function pattern allows for specialized implementations like DisputeKitShutter to include the justification in the hash while keeping the base functionality unchanged.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (2)
30-33: Unnecessary external self-call incurs gas & re-entrancy risk
this.castCommit(...)performs an external call to the same contract, paying
extra gas for ABI encoding/decoding and exposing the function to re-entrancy
checks (albeit low-risk here).Since
castCommitis declaredexternal, you can call it internally without
thisby using the fully-qualified function name:- this.castCommit(_coreDisputeID, _voteIDs, _commit); + castCommit(_coreDisputeID, _voteIDs, _commit);This is cheaper and avoids the external-call context switch.
85-92: Natspec parameter order does not match function signatureThe docstring lists the parameters as
(_choice, _justification, _salt)
while the signature is(_choice, _salt, _justification).
This can confuse automated doc generators.- * @param _justification The justification for the vote - * @param _salt A random salt for commitment + * @param _salt A random salt for commitment + * @param _justification The justification for the votecontracts/scripts/shutter.ts (2)
33-82: Missing retry / back-off on Shutter API call
fetchShutterDatafails the whole encryption flow on the first transient HTTP
error. Consider adding a simple exponential back-off or at least one retry to
improve UX when the Shutter API momentarily flakes.Not blocking, but worth considering.
160-186: Leaking raw Shutter keys in logs
encrypt()logs the eon key, identity and sigma to stdout. These are
cryptographically sensitive materials and should not be printed in production
mode.Recommend gating the
console.logstatements behind an environment variable
(e.g.DEBUG) or removing them entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
contracts/package.json(1 hunks)contracts/scripts/shutter.ts(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (8)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
30-30: Version update from 0.8.0 to 0.9.0The version constant has been updated as part of the broader Shutter integration changes across dispute kit contracts.
contracts/package.json (1)
159-160:❓ Verification inconclusive
Added Shutter SDK and related dependencies
These new dependencies are required to support the Shutter Network integration for encrypted commitments:
@shutter-network/shutter-sdk- Core SDK for Shutter Network functionalityisomorphic-fetch- Cross-environment fetch implementationNote that the Shutter SDK is at version
0.0.1, which suggests it's in early development.
🏁 Script executed:
#!/bin/bash # Check for newer versions of the Shutter SDK and potential issues echo "Checking for latest version of @shutter-network/shutter-sdk" npm view @shutter-network/shutter-sdk versions echo "Checking for any reported issues or vulnerabilities" npm audit --json | grep -i shutterLength of output: 682
🏁 Script executed:
#!/bin/bash # Generate a lockfile to enable auditing npm install --package-lock-only echo "Re-checking for any reported issues or vulnerabilities after lockfile creation" npm audit --json | grep -i shutterLength of output: 796
Verify Shutter SDK and Fetch Dependency
The PR adds:
@shutter-network/shutter-sdk@ 0.0.1isomorphic-fetch@ ^3.0.0Because this project uses workspace-protocols, running
npm auditin CI failed—please manually:
• Confirm there are no known vulnerabilities in these packages (e.g. via a localnpm auditoryarn audit).
• Verify that 0.0.1 of the Shutter SDK is the intended stable release (bump when a newer stable version is available).contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
241-241: Improved parameter documentationThe
_commitparameter documentation is updated to more clearly indicate it's a "commitment hash" rather than a general commit.
286-286: Added vote hash calculation for verificationThe code now uses the new
hashVotefunction to generate a vote hash, which enables customized hashing strategies in derived contracts.
292-293: Updated vote hash verification logic and error messageThe verification logic now compares against the computed
voteHashinstead of directly hashing the choice and salt. The error message has been updated accordingly.contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
14-14: Version update from 0.8.0 to 0.9.0The version constant has been updated as part of the broader Shutter integration changes across dispute kit contracts.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
56-72: Event is emitted even whencastCommitrevertsBecause
castCommitShutteremitsCommitCastShutterafter the external call,
a failure insidecastCommitwill revert the whole transaction and the event
will not be emitted.
If your intention was to guarantee the event only when the commit succeeds,
this is fine. If you need to record the attempt, move theemitin a
try/catchblock.contracts/scripts/shutter.ts (1)
88-127: Error parsing assumes specific JSON structure
fetchDecryptionKeythrows ifjsonResponse.messageis undefined, but the
Shutter API returns the payload atjsonResponseroot according to the spec
used earlier. Align the two helpers or add a type-guard to avoid false
positives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/test/arbitration/index.ts (2)
65-65: Test function updated but consider expanding test coverageThe deployContracts function is properly updated to include DisputeKitShutter. However, while the deployment and registration of DisputeKitShutter are verified, there are no specific tests for its unique functionality related to Shutter integration.
Consider adding specific test cases for the DisputeKitShutter's encryption/decryption capabilities and its integration with the Shutter API to ensure complete coverage.
Also applies to: 71-71, 73-73
6-6: Update test description to reflect expanded scopeThe describe block is still labeled "DisputeKitClassic" even though the tests now cover both classic and shutter dispute kits. Consider renaming this to "DisputeKits" or "Arbitration System" to more accurately reflect the expanded test scope.
-describe("DisputeKitClassic", async () => { +describe("DisputeKits", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
contracts/README.md(1 hunks)contracts/deploy/00-home-chain-arbitration.ts(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter_Proxy.json(1 hunks)contracts/src/proxy/KlerosProxies.sol(1 hunks)contracts/test/arbitration/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- contracts/README.md
- contracts/src/proxy/KlerosProxies.sol
- contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter_Proxy.json
- contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
contracts/deploy/00-home-chain-arbitration.ts (1)
99-106: Good integration of DisputeKitShutterThe implementation properly deploys the new DisputeKitShutter contract as upgradeable and registers it with the core contract. This aligns with the PR objective of introducing Shutter API integration for message encryption in the dispute kit system.
Consider adding a brief comment explaining what DisputeKitShutter is for and how it differs from DisputeKitClassic, which would improve code documentation.
contracts/test/arbitration/index.ts (4)
3-3: LGTM!Correctly imports the new DisputeKitShutter type.
9-9: LGTM!Properly declares the disputeKitShutter variable for use in tests.
13-13: LGTM!Updated to destructure the disputeKitShutter from the return value of deployContracts().
18-18: LGTM!Test correctly verifies both dispute kits are created with the expected IDs and addresses.
Also applies to: 21-22
…ring feat: shutter support in dispute commiting & appeal
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
web/src/pages/Cases/CaseDetails/Voting/index.tsx (1)
58-58: Consider making the dispute prop required.The
disputeprop is marked as optional in the interface but appears to be essential for Shutter dispute kit functionality.If the dispute data is always expected to be available when this component is rendered, consider making it required:
interface IVoting { arbitrable?: `0x${string}`; currentPeriodIndex: number; - dispute: DisputeDetailsQuery["dispute"]; + dispute: NonNullable<DisputeDetailsQuery["dispute"]>; }Or add runtime validation if it should remain optional.
web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx (1)
1-17: Consider adding JSDoc documentation for the new component.This is a new component with specific business logic for the Shutter dispute kit. Adding documentation would help other developers understand its purpose and usage.
Add JSDoc documentation:
+/** + * Shutter voting component that handles the commit and reveal phases + * for disputes using the Shutter dispute kit. + * + * @param arbitrable - The arbitrable contract address + * @param setIsOpen - Function to control popup visibility + * @param dispute - The dispute data from the query + * @param currentPeriodIndex - The current period index of the dispute + */ const Shutter: React.FC<IShutter> = ({ arbitrable, setIsOpen, dispute, currentPeriodIndex }) => {web/src/pages/Cases/CaseDetails/Appeal/Shutter/index.tsx (2)
8-9: Consider code reuse opportunities with the Classic appeal component.The component imports
Optionsfrom the Classic appeal but implements its ownFundcomponent. This might indicate code duplication or inconsistent patterns.Consider whether the appeal components can share more common logic:
- Shared base component: Create a common base that both Classic and Shutter can extend
- Shared utility functions: Extract common state management and popup logic
- Consistent component structure: Ensure both appeal types follow the same patterns
This would improve maintainability and reduce the risk of divergent behavior between dispute kit types.
1-49: Consider adding PropTypes or more detailed TypeScript interfaces.The component interface is minimal but could benefit from more detailed type definitions for better development experience and error catching.
Enhance the interface with more detailed types:
interface IShutter { + /** Controls visibility of the appeal mini guide */ isAppealMiniGuideOpen: boolean; + /** Function to toggle the appeal mini guide visibility */ toggleAppealMiniGuide: () => void; }Additionally, consider if the component needs any error boundaries or loading states for better user experience.
web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx (1)
52-62: Consider simplifying the useNeedFund logic.The conditional logic is complex and could benefit from clearer variable naming or breaking into smaller functions for better readability.
const useNeedFund = () => { const { loserSideCountdown } = useCountdownContext(); const { fundedChoices, winningChoice } = useFundingContext(); + + const hasActiveLoserCountdown = (loserSideCountdown ?? 0) > 0; + const hasUnfundedWinningChoice = !isUndefined(fundedChoices) && + !isUndefined(winningChoice) && + fundedChoices.length > 0 && + !fundedChoices.includes(winningChoice); + - return ( - (loserSideCountdown ?? 0) > 0 || - (!isUndefined(fundedChoices) && - !isUndefined(winningChoice) && - fundedChoices.length > 0 && - !fundedChoices.includes(winningChoice)) - ); + return hasActiveLoserCountdown || hasUnfundedWinningChoice; };web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (1)
85-86: Consider security implications of localStorage for vote data.Storing vote choice, salt, and justification in localStorage could pose privacy risks if the device is compromised. Consider if this sensitive data needs additional protection.
While localStorage is convenient for persistence, consider:
- Adding encryption for the stored vote data
- Implementing automatic cleanup after successful reveal
- Warning users about device security when storing sensitive voting information
The current implementation does clean up on successful reveal (line 83 in Reveal.tsx), which is good practice.
web/src/utils/shutter.ts (1)
33-33: Consider making API URL configurable.The Shutter API URL is hardcoded, which makes it difficult to use different environments (development, staging, production) or handle API endpoint changes.
+ const SHUTTER_API_BASE_URL = process.env.REACT_APP_SHUTTER_API_URL || "https://shutter-api.shutter.network"; + async function fetchShutterData(decryptionTimestamp: number): Promise<ShutterApiMessageData> { try { const identityPrefix = generateRandomBytes32(); - const response = await fetch("https://shutter-api.shutter.network/api/register_identity", { + const response = await fetch(`${SHUTTER_API_BASE_URL}/api/register_identity`, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
web/package.json(2 hunks)web/src/hooks/queries/useDisputeDetailsQuery.ts(1 hunks)web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Appeal/Shutter/index.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Appeal/index.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Timeline.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Vote.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/OptionsContainer.tsx(2 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Shutter/index.tsx(1 hunks)web/src/pages/Cases/CaseDetails/Voting/index.tsx(5 hunks)web/src/pages/Cases/CaseDetails/index.tsx(1 hunks)web/src/utils/shutter.ts(1 hunks)web/vite.config.js(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- web/src/pages/Cases/CaseDetails/Voting/Classic/Vote.tsx
- web/src/pages/Cases/CaseDetails/Voting/Classic/Commit.tsx
- web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx
- web/src/hooks/queries/useDisputeDetailsQuery.ts
- web/package.json
🧰 Additional context used
🧠 Learnings (1)
web/vite.config.js (1)
Learnt from: kemuru
PR: kleros/kleros-v2#1994
File: web/vite.config.js:26-33
Timestamp: 2025-05-23T17:47:39.947Z
Learning: The viteStaticCopy plugin configuration in web/vite.config.js correctly copies Shutter SDK files from the installed node_modules location. The path resolve(__dirname, "../node_modules/@shutter-network/shutter-sdk/dist/*") works when dependencies are properly installed, despite initial analysis suggesting otherwise due to sandbox environment limitations.
🧬 Code Graph Analysis (3)
web/src/pages/Cases/CaseDetails/Voting/index.tsx (2)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
DisputeDetailsQuery(8-8)web/src/consts/index.ts (1)
getDisputeKitName(46-56)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx (1)
web-devtools/src/hooks/useLocalStorage.ts (1)
useLocalStorage(3-22)
web/src/pages/Cases/CaseDetails/Voting/Shutter/Commit.tsx (4)
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
DisputeDetailsQuery(8-8)web-devtools/src/hooks/useLocalStorage.ts (1)
useLocalStorage(3-22)web/src/pages/Cases/CaseDetails/Timeline.tsx (1)
getDeadline(143-154)web/src/utils/shutter.ts (1)
encrypt(149-173)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Mend Security Check
🔇 Additional comments (14)
web/src/pages/Cases/CaseDetails/index.tsx (1)
82-82: LGTM! Clean integration for Shutter dispute kit support.Adding the
disputeprop to the Voting component enables conditional rendering between Classic and Shutter voting interfaces based on the dispute kit type. This change aligns well with the broader Shutter dispute kit integration.web/src/pages/Cases/CaseDetails/Timeline.tsx (1)
143-154: Good refactoring for code reusability.Exporting the
getDeadlinefunction enables its reuse across the codebase, particularly for keeper bots and scripts that need consistent deadline calculations for Shutter dispute kit functionality. The function logic remains unchanged and correctly calculates deadlines based on period changes.web/src/pages/Cases/CaseDetails/Voting/OptionsContainer.tsx (1)
73-83: Excellent error handling improvement.The try-catch-finally block ensures proper state cleanup regardless of whether
handleSelectionsucceeds or fails. The simplified dependency array is also correct sincehandleSelectionis the only unstable dependency. This robust error handling is especially important for the voting flow in the new Shutter dispute kit.web/vite.config.js (2)
5-9: Necessary imports for Shutter SDK integration.The added imports support the viteStaticCopy plugin configuration and ES module compatibility for path resolution.
26-33: Correct Shutter SDK static copy configuration.Based on previous validation in the retrieved learnings, this configuration correctly copies Shutter SDK files from the installed node_modules location to support frontend encryption/decryption functionality. The path resolution and plugin configuration are appropriate for the Shutter dispute kit integration.
web/src/pages/Cases/CaseDetails/Appeal/Shutter/index.tsx (1)
2-2:✅ Verification successful
Verify context compatibility between Classic and Shutter appeal flows.
The component uses
useSelectedOptionContextfrom the classic appeal context, but it's unclear if this context is appropriate for Shutter appeals or if they have different option structures.Verify that the classic appeal context is compatible with Shutter appeal requirements:
🏁 Script executed:
#!/bin/bash # Description: Check if the selected option context is used consistently across appeal components # and verify the Options component structure for compatibility # Find the classic appeal context implementation ast-grep --pattern 'export const $_ = createContext' # Check how Options component is used in both Classic and Shutter appeals rg -A 10 "Options.*setAmount" --type tsxLength of output: 125
I’ll dig deeper into the context and Options usage to confirm compatibility.
🏁 Script executed:
#!/bin/bash # 1. Locate where useSelectedOptionContext is defined rg -n "export (const|function) useSelectedOptionContext" -g "*.ts*" -A5 # 2. Find all createContext calls to see the context’s shape rg -n "createContext" -g "*.ts" -A5 # 3. Find the Options component implementation rg -n "export (const|function) Options" -g "*.tsx" -A5 # 4. See how Options is used in the Classic appeal flow rg -n "<Options" -g "*.tsx" | grep -i Classic # 5. See how Options is used in the Shutter appeal flow rg -n "<Options" -g "*.tsx" | grep -i ShutterLength of output: 1791
I’ll inspect the context definitions and the Options component implementation to confirm they align for Shutter.
🏁 Script executed:
#!/bin/bash # 1. Locate all createContext calls (to see context shapes) rg -n "createContext" -g "*.{ts,tsx}" -A5 # 2. Show SelectedOptionContext and OptionsContext definitions rg -n "SelectedOptionContext" -g "web/src/hooks/useClassicAppealContext.tsx" -A10 rg -n "OptionsContext" -g "web/src/hooks/useClassicAppealContext.tsx" -A10 # 3. Inspect the Classic Options component implementation rg -n "export const Options" -g "web/src/pages/Cases/CaseDetails/Appeal/Classic/Options/*.tsx" -A20 # 4. Inspect Shutter index usage of useSelectedOptionContext rg -n "useSelectedOptionContext" -g "web/src/pages/Cases/CaseDetails/Appeal/Shutter/index.tsx" -A20Length of output: 21940
Gathering where the provider is defined and how Classic wraps its children:
🏁 Script executed:
#!/bin/bash # 1. Find the exported Provider in the classic context file rg -n "export .*Provider" -g "web/src/hooks/useClassicAppealContext.tsx" # 2. Inspect the top of the Classic appeal index to see if it uses that Provider sed -n '1,60p' web/src/pages/Cases/CaseDetails/Appeal/Classic/index.tsxLength of output: 1954
Context compatibility confirmed
Both Classic and Shutter flows share the same SelectedOptionContext shape and OptionsContext, and the Options component’s API (accepting setAmount and reading the selected option) is identical in each. Shutter’s use ofselectedOption?.titlemirrors Classic’s popup logic, so no context changes are needed.web/src/pages/Cases/CaseDetails/Voting/Shutter/Reveal.tsx (3)
40-52: Good defensive programming with data validation.The parsing logic properly validates the required fields (
salt,choice,justification) and includes comprehensive error handling. This prevents runtime errors when localStorage contains malformed data.
71-93: Well-structured async operation with proper error handling.The reveal handling includes appropriate loading states, error checking, and cleanup operations. The transaction flow with toast notifications provides good user feedback.
33-33:✅ Verification successful
Verify the useLocalStorage hook signature.
The code destructures three values
[storedData, _, removeStoredData]fromuseLocalStorage, but the relevant code snippet shows the hook only returns two values[storedValue, setValue]. Please verify that the actual hook implementation in this project supports a third value for removal functionality.
🏁 Script executed:
#!/bin/bash # Search for the useLocalStorage hook implementation to verify its signature rg -A 10 "export.*useLocalStorage" --type typescriptLength of output: 89
Let’s locate where
useLocalStorageis imported and defined in the repo:
🏁 Script executed:
#!/bin/bash # Find all references to useLocalStorage to identify its import location and implementation rg -n "useLocalStorage" .Length of output: 2685
useLocalStoragedestructuring is correctYou’re importing
useLocalStoragefromreact-use, which returns a three-element tuple[value, setValue, removeValue]. The third element is indeed the removal function, so the[storedData, _, removeStoredData]destructuring is valid. No changes required here.web/src/pages/Cases/CaseDetails/Appeal/Shutter/Fund.tsx (1)
94-110: Robust input validation and user experience.The debouncing, balance checking, and comprehensive disable conditions provide excellent user experience and prevent common input errors.
web/src/utils/shutter.ts (4)
28-72: Excellent error handling and validation.The function includes comprehensive error handling for different failure scenarios, proper JSON parsing validation, and meaningful error messages. The response validation ensures required data is present.
137-142: Secure random number generation.Good use of
crypto.getRandomValues()for generating cryptographically secure random bytes. The implementation correctly converts to hex format.
99-108: User-friendly error handling for timing constraints.The specific handling of "timestamp not reached yet" errors provides clear guidance to users about when they can attempt decryption again. This improves the user experience significantly.
149-173: Verify decryption delay calculation consistency.Ensure that the decryption delay calculation here aligns with the usage in the Commit component, particularly the 300-second buffer addition mentioned in Commit.tsx line 91.
The
encryptfunction acceptsdecryptionDelayas a parameter and adds it to the current timestamp. Verify that callers are consistently handling the timing calculations, especially the buffer time mentioned in the Commit component.
and support for the Shutter testnet API endpoint
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
contracts/scripts/keeperBotShutter.ts (4)
40-48:⚠️ Potential issueAdd input validation to prevent runtime crashes.
The
decodefunction lacks validation for the message format, which can cause runtime errors if the message is malformed.Apply this diff to add proper validation:
-const decode = (message: string) => { - const SEPARATOR = "-"; - const [choice, salt, justification] = message.split(SEPARATOR); - return { - choice: BigInt(choice), - salt, - justification, - }; -}; +const decode = (message: string) => { + const SEPARATOR = "-"; + const parts = message.split(SEPARATOR); + if (parts.length !== 3) { + throw new Error(`Malformed decrypted message (${message})`); + } + const [choice, salt, justification] = parts; + try { + return { + choice: BigInt(choice), + salt, + justification, + }; + } catch (e) { + throw new Error(`Invalid choice value in message (${message}): ${e}`); + } +};
55-58:⚠️ Potential issueGuard against malformed Graph vote IDs.
Number()can silently yieldNaN, propagating bad data downstream.Apply this diff to add validation:
-const parseGraphVoteId = (graphVoteId: string) => { - const [disputeKitID, localDisputeID, localRoundID, voteID] = graphVoteId.split("-").map(Number); - return { disputeKitID, localDisputeID, localRoundID, voteID }; -}; +const parseGraphVoteId = (graphVoteId: string) => { + const [dk, ld, lr, v] = graphVoteId.split("-").map((n) => { + const parsed = Number(n); + if (Number.isNaN(parsed)) throw new Error(`Invalid graphVoteId segment "${n}" in "${graphVoteId}"`); + return parsed; + }); + return { disputeKitID: dk, localDisputeID: ld, localRoundID: lr, voteID: v }; +};
169-177:⚠️ Potential issuePrevent undefined access when no current local round exists.
filteredLocalRounds[0]will throw whenlocalRoundsis empty, causing the script to abort.Apply this diff to add safety checks:
- const disputeVotes = filteredRounds.map((round) => { - const dk = round.dispute.disputeKitDispute[0]; - const idx = Number(dk.currentLocalRoundIndex); - const filteredLocalRounds = dk.localRounds.filter((_, i) => i === idx); - return { - coreDispute: dk.coreDispute, - votes: filteredLocalRounds[0].votes, - }; - }); + const disputeVotes = filteredRounds.map((round) => { + const dk = round.dispute.disputeKitDispute[0]; + const idx = Number(dk.currentLocalRoundIndex); + const currentLocalRound = dk.localRounds[idx]; + if (!currentLocalRound) return null; // skip gracefully + return { + coreDispute: dk.coreDispute, + votes: currentLocalRound.votes, + }; + }).filter(Boolean);
213-219:⚠️ Potential issueFix event filter to match indexed bytes32 parameter.
getBytes(vote.commit)may break indexed event lookup if it returns a byte array that doesn't match the topic encoding.Apply this diff to fix the filter:
- const filter = disputeKitShutter.filters.CommitCastShutter(coreDispute.id, vote.juror.id, getBytes(vote.commit)); + const filter = disputeKitShutter.filters.CommitCastShutter(coreDispute.id, vote.juror.id, vote.commit);contracts/scripts/shutter.ts (1)
156-161:⚠️ Potential issueFix Node.js compatibility issue with crypto API.
crypto.getRandomValuesis a Web Crypto API that's not available in Node.js environments.Apply this diff to use Node.js crypto instead:
-function generateRandomBytes32(): `0x${string}` { - return ("0x" + - crypto - .getRandomValues(new Uint8Array(32)) - .reduce((acc, byte) => acc + byte.toString(16).padStart(2, "0"), "")) as Hex; -} +function generateRandomBytes32(): `0x${string}` { + return ("0x" + crypto.randomBytes(32).toString("hex")) as Hex; +}
🧹 Nitpick comments (1)
contracts/scripts/keeperBotShutter.ts (1)
298-298: Remove unnecessary continue statement.The continue statement at the end of the catch block is unnecessary as it's the last statement in the loop iteration.
- continue;🧰 Tools
🪛 Biome (1.9.4)
[error] 298-298: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/scripts/keeperBot.ts(8 hunks)contracts/scripts/keeperBotShutter.ts(1 hunks)contracts/scripts/shutter.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
contracts/scripts/keeperBotShutter.ts
[error] 298-298: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: contracts-testing
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (5)
contracts/scripts/keeperBot.ts (3)
91-116: Excellent dynamic dispute kit resolution implementation!The
getDisputeKitfunction properly handles the transition from hardcoded dispute kit usage to dynamic resolution based on the dispute kit address. This enables seamless support for both Classic and Shutter dispute kits within the same keeper bot.
598-612: Well-integrated auto-reveal functionality for Shutter disputes.The auto-reveal section is properly placed in the workflow and includes proper period transition handling. The
SHUTTER_AUTO_REVEAL_ONLYflag provides useful operational flexibility for running dedicated auto-reveal instances.
374-431: Properly updated withdrawal logic for dynamic dispute kits.The
withdrawAppealContributionfunction correctly uses the dynamically resolved dispute kit and local IDs, ensuring compatibility with both Classic and Shutter dispute kits.contracts/scripts/shutter.ts (2)
168-194: Well-structured encryption function with comprehensive error handling.The
encryptfunction properly handles the Shutter API workflow, including timestamp calculation, data fetching, format validation, and encryption. The logging provides good visibility into the encryption process.
202-215: Robust decryption implementation with proper error handling.The
decryptfunction correctly handles the decryption workflow with appropriate error handling and format validation. The hex string conversion is properly handled.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
contracts/scripts/keeperBotShutter.ts (4)
40-48: Add basic validation todecode()to avoid runtime crashesIf an unexpected message arrives (missing separator / wrong format),
BigInt(choice)will throw and bring the whole bot down.
55-58: Guard against malformed Graph vote IDs
Number()can silently yieldNaN, propagating bad data down-stream. Add validation and a clear error message.
169-177: Possibleundefinedaccess when no current local round exists
filteredLocalRounds[0]will throw whenlocalRoundsis empty, causing the whole script to abort.
215-221:getBytes(vote.commit)may break indexed event lookup
_commitis likely an indexedbytes32. Passing a byte array returned bygetBytesmay not match the topic encoding, leading to empty event sets and missed reveals.
🧹 Nitpick comments (2)
contracts/scripts/keeperBotShutter.ts (2)
300-300: Remove unnecessary continue statement.The continue statement at the end of the catch block is unnecessary since it's the last statement in the loop iteration.
- continue;🧰 Tools
🪛 Biome (1.9.4)
[error] 300-300: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
309-309: Consider making dispute skip list configurable.The hardcoded dispute IDs to skip reduce maintainability. Consider using an environment variable or configuration file.
+const DISPUTES_TO_SKIP = env.optional("DISPUTES_TO_SKIP", "44,45,51,54").split(","); - await shutterAutoReveal(disputeKitShutter, ["44", "45", "51", "54"]); + await shutterAutoReveal(disputeKitShutter, DISPUTES_TO_SKIP);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/scripts/keeperBotShutter.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
contracts/scripts/keeperBotShutter.ts
[error] 300-300: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
🔇 Additional comments (1)
contracts/scripts/keeperBotShutter.ts (1)
1-34: Well-structured imports and configuration setup.The imports, environment variable handling, and contract retrieval logic are properly implemented with appropriate error handling.
|
Code Climate has analyzed commit d43e68e and detected 124 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
web/src/utils/shutter.ts (2)
45-46: Remove incomplete logging commentThere's an incomplete comment about logging the response status with no actual implementation.
- // Log the response status - +
123-131: Add JSDoc documentation for helper functionsThe helper functions lack documentation which would improve maintainability.
+/** + * Ensures a string is a valid hex string with 0x prefix + * @param hexString The hex string to validate + * @returns The validated hex string with 0x prefix + * @throws Error if hexString is undefined, null, or invalid + */ function ensureHexString(hexString: string | undefined): `0x${string}` {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/.env.example(1 hunks)web/.env.devnet-neo.public(1 hunks)web/.env.devnet-university.public(1 hunks)web/.env.devnet.public(1 hunks)web/.env.local.public(1 hunks)web/.env.mainnet-neo.public(1 hunks)web/.env.testnet.public(1 hunks)web/src/utils/shutter.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- web/.env.testnet.public
- web/.env.devnet-neo.public
- web/.env.devnet.public
- web/.env.local.public
- web/.env.devnet-university.public
- web/.env.mainnet-neo.public
- contracts/.env.example
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
🔇 Additional comments (2)
web/src/utils/shutter.ts (2)
1-21: LGTM: Well-structured interfaces and importsThe imports and interface definitions are clean and properly typed. The interfaces correctly model the Shutter API response structure with appropriate optional fields.
137-142:✅ Verification successful
Verify cryptographic randomness availability
The function uses
crypto.getRandomValueswithout checking if it's available. Consider adding a fallback or validation.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any compatibility concerns with crypto.getRandomValues in the codebase # Expected: Find usage patterns and any existing fallbacks rg -A 3 -B 3 "crypto\.getRandomValues|window\.crypto" --type ts --type tsx --type js --type jsxLength of output: 125
🏁 Script executed:
#!/bin/bash # Re-run search without unsupported --type flags, targeting relevant file extensions rg -A 3 -B 3 'crypto\.getRandomValues|window\.crypto' \ --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx'Length of output: 113
No fallback needed for crypto.getRandomValues in browser code
After searching the repository, there are no other uses of
crypto.getRandomValuesor any existing fallbacks—and since this utility lives underweb/src, it targets modern browsers wherecrypto.getRandomValuesis universally available. No changes required here.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/src/components/Popup/Description/VoteWithCommit.tsx(1 hunks)web/src/components/Popup/ExtraInfo/VoteWithCommitExtraInfo.tsx(1 hunks)web/src/components/Popup/index.tsx(4 hunks)web/src/pages/Cases/CaseDetails/Voting/index.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/pages/Cases/CaseDetails/Voting/index.tsx
🧰 Additional context used
🧠 Learnings (3)
web/src/components/Popup/ExtraInfo/VoteWithCommitExtraInfo.tsx (6)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:125-127
Timestamp: 2024-10-29T10:14:52.512Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when `isEmailUpdateable` is false, `user?.emailUpdateableAt` is always defined. Therefore, using the non-null assertion `!` with `user?.emailUpdateableAt!` is acceptable because TypeScript may not infer its definiteness.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:69-69
Timestamp: 2024-10-29T10:13:04.524Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when the button is disabled, the associated logic won't be reached, and certain code paths may exist for TypeScript purposes.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1582
File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:88-90
Timestamp: 2024-10-09T10:22:41.474Z
Learning: Next.js recommends using the `useEffect` hook to set `isClient` and using `suppressHydrationWarning` as a workaround for handling hydration inconsistencies, especially when dealing with data like `knownArbitrables` that may differ between server-side and client-side rendering. This approach is acceptable in TypeScript/React applications, such as in `web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx`.
web/src/components/Popup/Description/VoteWithCommit.tsx (3)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:125-127
Timestamp: 2024-10-29T10:14:52.512Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when `isEmailUpdateable` is false, `user?.emailUpdateableAt` is always defined. Therefore, using the non-null assertion `!` with `user?.emailUpdateableAt!` is acceptable because TypeScript may not infer its definiteness.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
web/src/components/Popup/index.tsx (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
🧬 Code Graph Analysis (1)
web/src/components/Popup/Description/VoteWithCommit.tsx (1)
web/src/components/Popup/index.tsx (1)
VoteDescriptionEmphasizedDate(111-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
🔇 Additional comments (4)
web/src/components/Popup/ExtraInfo/VoteWithCommitExtraInfo.tsx (1)
15-24: LGTM! Clean implementation of conditional messaging.The addition of the
automaticVoteRevealprop with proper TypeScript interface and default handling is well-implemented. The conditional messaging appropriately differentiates between automatic and manual vote reveal scenarios for the Shutter dispute kit integration.web/src/components/Popup/index.tsx (2)
171-171: LGTM! Proper interface extension for Shutter integration.The addition of the optional
automaticVoteRevealproperty to theIPopupinterface is correctly implemented and follows TypeScript best practices.
183-183: LGTM! Proper prop threading to child components.The
automaticVoteRevealprop is correctly passed through the component hierarchy to bothVoteWithCommitandVoteWithCommitExtraInfocomponents, enabling consistent behavior for the Shutter dispute kit's automatic vote reveal functionality.Also applies to: 218-218, 265-265
web/src/components/Popup/Description/VoteWithCommit.tsx (1)
6-7: LGTM! Proper interface definition for enhanced functionality.The interface correctly defines both props as optional, supporting the new automatic vote reveal functionality while maintaining backward compatibility.
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } catch (e) { | ||
| logger.error( | ||
| `CastVoteShutter: will fail for disputeID: ${coreDispute.id} and voteIDs [${voteIDs.join(", ")}], skipping` | ||
| ); | ||
| continue; | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
subgraph/core-neo/subgraph.template.yaml (1)
157-157: Incorrect mapping file forDisputeKitShutterin Neo template.The Neo Shutter data source reuses the Classic mapping file, which should point to the Shutter handler instead.
- file: ./src/DisputeKitClassic.ts + file: ./src/DisputeKitShutter.tscontracts/scripts/keeperBotShutter.ts (3)
59-62: Guard against malformed Graph vote IDs
Number()can silently yieldNaN, propagating bad data downstream. Add validation and a clear error message.- const [disputeKitID, localDisputeID, localRoundID, voteID] = graphVoteId.split("-").map(Number); - return { disputeKitID, localDisputeID, localRoundID, voteID }; + const parts = graphVoteId.split("-"); + if (parts.length !== 4) { + throw new Error(`Invalid graphVoteId format: "${graphVoteId}"`); + } + const [dk, ld, lr, v] = parts.map((n) => { + const parsed = Number(n); + if (Number.isNaN(parsed)) throw new Error(`Invalid graphVoteId segment "${n}" in "${graphVoteId}"`); + return parsed; + }); + return { disputeKitID: dk, localDisputeID: ld, localRoundID: lr, voteID: v };
173-181: Possibleundefinedaccess when no current local round exists
filteredLocalRounds[0]will throw whenlocalRoundsis empty, causing the whole script to abort.Add an existence check:
- const filteredLocalRounds = dk.localRounds.filter((_, i) => i === idx); - return { - coreDispute: dk.coreDispute, - votes: filteredLocalRounds[0].votes, - }; + const currentLocalRound = dk.localRounds[idx]; + if (!currentLocalRound) return null; // skip gracefully + return { + coreDispute: dk.coreDispute, + votes: currentLocalRound.votes, + };Then filter out null values from the resulting array:
- }); + }).filter(Boolean);
219-225:getBytes(vote.commit)may break indexed event lookup
_commitis likely an indexedbytes32. Passing a byte array returned bygetBytesmay not match the topic encoding, leading to empty event sets and missed reveals.- const filter = disputeKitShutter.filters.CommitCastShutter(coreDispute.id, vote.juror.id, getBytes(vote.commit)); + const filter = disputeKitShutter.filters.CommitCastShutter(coreDispute.id, vote.juror.id, vote.commit);contracts/scripts/shutter.ts (1)
172-177:crypto.getRandomValuesis not available in Node.js
getRandomValuesis a Web Crypto API. In a Node environment usecrypto.randomBytes.- return ("0x" + - crypto - .getRandomValues(new Uint8Array(32)) - .reduce((acc, byte) => acc + byte.toString(16).padStart(2, "0"), "")) as Hex; + return ("0x" + crypto.randomBytes(32).toString("hex")) as Hex;This prevents runtime crashes when the script is executed with
ts-node.
🧹 Nitpick comments (3)
contracts/.env.example (1)
21-22: Consider reordering environment variables alphabetically.The static analysis tool suggests that
SHUTTER_APIandSHUTTER_API_KEYshould be placed beforeSUBGRAPH_URLto maintain alphabetical ordering of environment variables.LOGTAIL_TOKEN_DISPUTOR_BOT=ABC123ABC123ABC123ABC HEARTBEAT_URL_KEEPER_BOT=https://uptime.betterstack.com/api/v1/heartbeat/ABC123ABC123ABC123ABC -SHUTTER_API=testnet -SHUTTER_API_KEY= DISPUTES_TO_SKIP= +SHUTTER_API=testnet +SHUTTER_API_KEY= +SUBGRAPH_URL=https://api.studio.thegraph.com/query/61738/kleros-v2-core-devnet/version/latestcontracts/scripts/keeperBotShutter.ts (2)
267-280: Improve error handling for failed vote simulations.The catch block logs the error but swallows the exception. Consider adding a comment to clarify this is intentional to continue processing other votes.
} catch (e) { logger.error( `CastVoteShutter: will fail for disputeID: ${coreDispute.id} and voteIDs [${voteIDs.join(", ")}], skipping` ); + // Continue processing other votes continue; }
302-305: Remove unnecessary continue statement.The
continuestatement at the end of the loop is redundant.} catch (e) { logger.error(e, "Failure"); - continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contracts/.env.example(1 hunks)contracts/package.json(1 hunks)contracts/scripts/keeperBotShutter.ts(1 hunks)contracts/scripts/shutter.ts(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(7 hunks)contracts/test/foundry/KlerosCore.t.sol(1 hunks)subgraph/core-neo/subgraph.template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/package.json
- contracts/test/foundry/KlerosCore.t.sol
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: kemuru
PR: kleros/kleros-v2#1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-08T16:23:56.291Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
Learnt from: kemuru
PR: kleros/kleros-v2#1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
subgraph/core-neo/subgraph.template.yaml (7)
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-5
Timestamp: 2024-11-19T17:18:39.007Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-17
Timestamp: 2024-11-19T16:09:41.467Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/requests/gqlClient.ts:18-18
Timestamp: 2024-10-24T08:16:02.749Z
Learning: In this TypeScript project, when a file (such as `kleros-sdk/src/requests/gqlClient.ts`) exports only a single entity, it's acceptable to use default exports instead of named exports.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1846
File: prettier-config/index.js:26-26
Timestamp: 2025-01-23T08:14:47.397Z
Learning: The prettier-plugin-solidity plugin is installed in the kleros-v2 repository, even though it's not visible in the sandbox environment's node_modules (which is expected as node_modules is not committed to the repository).
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (5)
Learnt from: kemuru
PR: kleros/kleros-v2#1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-08T16:23:56.291Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
Learnt from: kemuru
PR: kleros/kleros-v2#1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:38-40
Timestamp: 2024-10-21T10:32:16.970Z
Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1839
File: web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx:141-149
Timestamp: 2025-01-17T11:11:32.535Z
Learning: In the Kleros v2 codebase, using -1 as an initial value for choice tracking is preferred over undefined on the client-side to explicitly indicate that no option has been selected. This value is only used internally and never reaches the contract.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
contracts/.env.example (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1687
File: web/src/context/AtlasProvider.tsx:225-244
Timestamp: 2024-10-15T16:18:32.543Z
Learning: In `web/src/context/AtlasProvider.tsx`, the `atlasUri` variable comes from environment variables and does not change, so it does not need to be included in dependency arrays.
contracts/scripts/keeperBotShutter.ts (18)
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1762
File: web/src/utils/parseWagmiError.ts:10-17
Timestamp: 2024-11-29T06:23:15.955Z
Learning: In the `web/src/utils/parseWagmiError.ts` file and throughout the codebase, prefer using optional chaining to handle `undefined` or `null` values, including optional arrays, without adding explicit existence or length checks.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1727
File: web/src/utils/atlas/updateEmail.ts:34-37
Timestamp: 2024-10-28T12:20:19.884Z
Learning: In `web/src/utils/atlas/updateEmail.ts`, the error coming from the `GraphQLError` array already has the necessary structure, so additional specificity in error handling is unnecessary.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Learnt from: kemuru
PR: kleros/kleros-v2#1707
File: web/src/pages/Home/CourtOverview/BarChart.tsx:40-42
Timestamp: 2024-10-11T10:37:40.896Z
Learning: In `BarChart.tsx`, the function `getPercentValue` may return `NaN` if `chartData.total` is zero, but this is acceptable since there should always be data, and `chartData.total` should not be zero.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1582
File: web-devtools/src/app/(main)/ruler/RulingModes.tsx:179-199
Timestamp: 2024-10-09T10:18:51.089Z
Learning: In `web-devtools/src/app/(main)/ruler/RulingModes.tsx`, the `handleUpdate` function already handles errors via `wrapWithToast`, so additional error handling is unnecessary.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:43-43
Timestamp: 2024-10-14T15:29:32.954Z
Learning: In the Kleros SDK, when using the `populateTemplate` function in `kleros-sdk/src/utils/getDispute.ts`, missing variables in Mustache templates are acceptable. Mustache fills in blanks, and it's preferable to return the partially populated template without throwing errors, as it's still helpful for the consumer.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1755
File: kleros-app/src/lib/atlas/hooks/useSessionStorage.ts:3-12
Timestamp: 2024-11-21T05:38:11.576Z
Learning: In the `useSessionStorage` hook in `kleros-app/src/lib/atlas/hooks/useSessionStorage.ts`, the error handling in the `catch` block covers cases where `window` is undefined or `sessionStorage` throws an exception, so additional checks are unnecessary.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1794
File: web/src/hooks/useStarredCases.tsx:13-18
Timestamp: 2024-12-16T17:17:32.359Z
Learning: In `useStarredCases.tsx`, when handling the `starredCases` Map from local storage, direct mutation is acceptable to prevent the overhead of copying, provided it doesn't adversely affect React's render cycle.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1647
File: web/src/context/NewDisputeContext.tsx:0-0
Timestamp: 2024-10-08T16:23:56.290Z
Learning: The `delete` operator is used in the `constructDisputeTemplate` function in `web/src/context/NewDisputeContext.tsx` to remove the `policyURI` field if it is an empty string.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1647
File: web/src/context/NewDisputeContext.tsx:0-0
Timestamp: 2024-07-25T11:58:27.058Z
Learning: The `delete` operator is used in the `constructDisputeTemplate` function in `web/src/context/NewDisputeContext.tsx` to remove the `policyURI` field if it is an empty string.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:48-55
Timestamp: 2024-10-22T10:26:23.167Z
Learning: In the codebase, try-catch blocks are used to handle potential JSON parsing errors, even if the catch block rethrows the error.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/requests/fetchDisputeDetails.ts:0-0
Timestamp: 2024-10-24T08:09:39.086Z
Learning: When handling urql `CombinedError`, prefer to throw it directly without additional handling to avoid losing information.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1727
File: web/src/utils/atlas/updateEmail.ts:32-33
Timestamp: 2024-10-28T12:20:36.536Z
Learning: In the 'kleros-v2' project, it's acceptable to log raw error objects in production code, including in the `updateEmail` function within `web/src/utils/atlas/updateEmail.ts`.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1759
File: web/src/pages/Cases/CaseDetails/Appeal/Classic/Fund.tsx:140-143
Timestamp: 2024-11-26T10:50:23.399Z
Learning: In the Kleros-v2 React project, errors are handled by the `wrapWithToast` function, which displays error messages to users via toast notifications.
contracts/scripts/shutter.ts (8)
Learnt from: jaybuidl
PR: kleros/kleros-v2#1964
File: contracts/scripts/shutter.ts:3-3
Timestamp: 2025-04-30T22:12:25.476Z
Learning: In Node.js, importing the crypto module as `import crypto from "crypto"` is a valid approach when using methods like `crypto.randomBytes()`. This import style works fine and doesn't need to be changed to a destructured import when these methods are used.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1620
File: contracts/test/arbitration/draw.ts:98-98
Timestamp: 2024-11-05T11:32:29.452Z
Learning: In Ethers v6, `ethers.toBeHex` is the correct method to convert numbers to hex strings.
Learnt from: salgozino
PR: kleros/kleros-v2#1831
File: kleros-sdk/src/utils/getDispute.ts:59-70
Timestamp: 2025-01-13T19:30:33.915Z
Learning: When checking if a hex string represents zero in JavaScript, using `Number()` is safe as it correctly converts "0x0", "0x00" to 0. While `Number()` can have precision issues with large hex values, it's reliable for checking zero.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:48-55
Timestamp: 2024-10-22T10:26:23.167Z
Learning: In the codebase, try-catch blocks are used to handle potential JSON parsing errors, even if the catch block rethrows the error.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/requests/fetchDisputeDetails.ts:0-0
Timestamp: 2024-10-24T08:09:39.086Z
Learning: When handling urql `CombinedError`, prefer to throw it directly without additional handling to avoid losing information.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1755
File: kleros-app/src/lib/atlas/hooks/useSessionStorage.ts:3-12
Timestamp: 2024-11-21T05:38:11.576Z
Learning: In the `useSessionStorage` hook in `kleros-app/src/lib/atlas/hooks/useSessionStorage.ts`, the error handling in the `catch` block covers cases where `window` is undefined or `sessionStorage` throws an exception, so additional checks are unnecessary.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1727
File: web/src/utils/atlas/updateEmail.ts:32-33
Timestamp: 2024-10-28T12:20:36.536Z
Learning: In the 'kleros-v2' project, it's acceptable to log raw error objects in production code, including in the `updateEmail` function within `web/src/utils/atlas/updateEmail.ts`.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1759
File: web/src/pages/Cases/CaseDetails/Appeal/Classic/Fund.tsx:140-143
Timestamp: 2024-11-26T10:50:23.399Z
Learning: In the Kleros-v2 React project, errors are handled by the `wrapWithToast` function, which displays error messages to users via toast notifications.
🧬 Code Graph Analysis (1)
contracts/scripts/shutter.ts (1)
web/src/utils/shutter.ts (2)
encrypt(151-175)decrypt(183-195)
🪛 dotenv-linter (3.3.0)
contracts/.env.example
[warning] 21-21: [UnorderedKey] The SHUTTER_API key should go before the SUBGRAPH_URL key
[warning] 22-22: [UnorderedKey] The SHUTTER_API_KEY key should go before the SUBGRAPH_URL key
🪛 Biome (1.9.4)
contracts/scripts/keeperBotShutter.ts
[error] 304-304: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🪛 GitHub Check: SonarCloud
contracts/scripts/keeperBotShutter.ts
[notice] 275-280: Exceptions should not be ignored
Handle this exception or don't catch it at all.See more on SonarQube Cloud
contracts/scripts/shutter.ts
[notice] 93-95: Exceptions should not be ignored
Handle this exception or don't catch it at all.See more on SonarQube Cloud
[notice] 129-131: Exceptions should not be ignored
Handle this exception or don't catch it at all.See more on SonarQube Cloud
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Mend Security Check
🔇 Additional comments (5)
contracts/scripts/keeperBotShutter.ts (1)
40-52: LGTM! Proper validation added.The decode function now includes proper validation for the message format, preventing runtime errors from malformed messages.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
249-271: Well-structured refactoring of commit casting.The refactoring to use an internal
_castCommitfunction provides good extensibility for derived contracts while maintaining the same external interface.
467-481: Good extensibility design with the virtual hashVote function.The virtual
hashVotefunction provides a clean extension point for derived contracts to customize vote hashing logic. The documentation clearly explains that unused parameters may be used by overriding contracts.
637-671: Useful view functions added for querying dispute state.The new view functions
getNumberOfRounds,getLocalDisputeRoundID, and the enhancedgetVoteInfoprovide valuable query capabilities for both on-chain and off-chain consumers.contracts/scripts/shutter.ts (1)
41-52: Secure API key handling implementation.The function properly handles API authentication by:
- Only adding the bearer token for mainnet
- Checking if the API key exists and is not empty after trimming
- Using appropriate header structure
| @@ -38,6 +38,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi | |||
| uint256 feeRewards; // Sum of reimbursable appeal fees available to the parties that made contributions to the ruling that ultimately wins a dispute. | |||
| uint256[] fundedChoices; // Stores the choices that are fully funded. | |||
| uint256 nbVotes; // Maximal number of votes this dispute can get. | |||
| //mapping(address drawnAddress => bool) alreadyDrawn; // DEPRECATED: DO NOT COMMIT | |||
There was a problem hiding this comment.
Remove deprecated code marked as "DO NOT COMMIT".
This line contains a deprecated mapping that is explicitly marked as "DO NOT COMMIT". It should be removed before merging.
- //mapping(address drawnAddress => bool) alreadyDrawn; // DEPRECATED: DO NOT COMMIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //mapping(address drawnAddress => bool) alreadyDrawn; // DEPRECATED: DO NOT COMMIT |
🤖 Prompt for AI Agents
In contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol at line 41,
remove the entire commented-out deprecated mapping line marked with "DO NOT
COMMIT" to clean up the code before merging.
Devnet contract deployed
DisputeKitShutter
DisputeKitBase changes
hashVote()which is overridden byDisputeKitShutterto include the justification in the commitment hash_castVote()with an arbitrary_jurorparameter to allow anyone to reveal a committed vote (which reverts if hash verification fails)getNumberOfRounds(_localDisputeID)getLocalDisputeRoundID(_coreDisputeID, _coreRoundID) returns (localDisputeID, localRoundID)PR-Codex overview
This PR introduces the
DisputeKitShutter, enhancing the voting system with new features, adjustments to existing components, and updates to the environment configurations. It aims to integrate Shutter-specific functionalities while improving the overall architecture.Detailed summary
DisputeKitShuttercontract implementing Shutter-specific voting features.VotingandAppealcomponents to handle Shutter disputes.useDisputeDetailsQueryto include new dispute kit details.REACT_APP_SHUTTER_API.VoteWithCommitandVoteWithCommitExtraInfo.DisputeKitShutter.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests